refactor: add time window for trace get method in span store interface #6242
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rim99
force-pushed
the
refactor-span-store-interface-add-time-window
branch
from
fcd8538 to
5e2d862
Compare
dosubot
bot
added
area/storage
enhancement
storage/badger
4 tasks
yurishkuro
reviewed
Nov 23, 2024
| @@ -266,7 +266,10 @@ func (aH *APIHandler) tracesByIDs(ctx context.Context, traceIDs []model.TraceID) | |||
| var traceErrors []structuredError | |||
| retMe := make([]*model.Trace, 0, len(traceIDs)) | |||
| for _, traceID := range traceIDs { | |||
| if trc, err := aH.queryService.GetTrace(ctx, traceID); err != nil { | |||
| query := spanstore.GetTraceParameters{ | |||
There was a problem hiding this comment.
somewhere here the timestamps must be handled, so leave a TODO. This could be another PR to precede this one, where we introduce timestamps params in /api/trace/{traceID} API.
There was a problem hiding this comment.
how is this comment resolved?
| @@ -136,7 +136,10 @@ func (h *HTTPGateway) getTrace(w http.ResponseWriter, r *http.Request) { | |||
| if h.tryParamError(w, err, paramTraceID) { | |||
| return | |||
| } | |||
| trc, err := h.QueryService.GetTrace(r.Context(), traceID) | |||
| query := spanstore.GetTraceParameters{ | |||
| TraceID: traceID, | |||
There was a problem hiding this comment.
copy timestamps from request
There was a problem hiding this comment.
Added TODO, since API changes will be in another PR: #6248
| gw.getTracesAndVerify(t, "/api/v3/traces/"+traceID.String(), traceID) | ||
| trace, query := makeTestTrace() | ||
| gw.reader.On("GetTrace", matchContext, query).Return(trace, nil).Once() | ||
| gw.getTracesAndVerify(t, "/api/v3/traces/"+query.TraceID.String(), query.TraceID) |
There was a problem hiding this comment.
could introduce a preliminary PR that adds timestamps args to the REST API calls
There was a problem hiding this comment.
that PR doesn't seem to compile, can you get it to green state?
There was a problem hiding this comment.
Here is a PR contains changes of current PR, and the PR above: #6258
rim99
force-pushed
the
refactor-span-store-interface-add-time-window
branch
from
5e2d862 to
5610442
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6242 +/- ##
==========================================
+ Coverage 96.10% 96.14% +0.03%
==========================================
Files 360 360
Lines 20429 20475 +46
==========================================
+ Hits 19634 19685 +51
+ Misses 607 603 -4
+ Partials 188 187 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
rim99
force-pushed
the
refactor-span-store-interface-add-time-window
branch
3 times, most recently
from
289e0a8 to
7c37eee
Compare
yurishkuro
added
the
changelog:new-feature
yurishkuro
reviewed
Nov 24, 2024
yurishkuro
reviewed
Nov 24, 2024
yurishkuro
reviewed
Nov 24, 2024
yurishkuro
reviewed
Nov 24, 2024
yurishkuro
reviewed
Nov 24, 2024
rim99
force-pushed
the
refactor-span-store-interface-add-time-window
branch
from
fd25a38 to
16da94c
Compare
|
PR updated, please review, thanks @yurishkuro |
rim99
force-pushed
the
refactor-span-store-interface-add-time-window
branch
from
56f361a to
5edb49b
Compare
yurishkuro
reviewed
Nov 26, 2024
Comment on lines
109
to
110
| for _, input := range inputs { | ||
| tsc := newTestServerClient(t) |
There was a problem hiding this comment.
Suggested change
| for _, input := range inputs { | |
| tsc := newTestServerClient(t) | |
| for _, input := range inputs { | |
| t.Run(input.name, func(t *testing.T) { | |
| tsc := newTestServerClient(t) | |
| ... | |
| }) |
Signed-off-by: rim99 <[email protected]>
Signed-off-by: rim99 <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Zhang Xin <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Zhang Xin <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Zhang Xin <[email protected]>
Signed-off-by: rim99 <[email protected]>
Signed-off-by: rim99 <[email protected]>
Signed-off-by: rim99 <[email protected]>
Signed-off-by: rim99 <[email protected]>
Signed-off-by: rim99 <[email protected]>
Signed-off-by: rim99 <[email protected]>
rim99
force-pushed
the
refactor-span-store-interface-add-time-window
branch
from
af0dee9 to
491c826
Compare
|
@rim99 could you please resolve the conflicts? |
4 tasks
…nterface-add-time-window
Signed-off-by: Zhang Xin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Part of #4150
Description of the changes
Refactor trace get method in span store interface:
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test